Skip to content

Refactor occupy.h/cpp and update MPI kmesh_world - #7906

Open
mohanchen wants to merge 25 commits into
deepmodeling:developfrom
mohanchen:2026-09-04-line1
Open

Refactor occupy.h/cpp and update MPI kmesh_world#7906
mohanchen wants to merge 25 commits into
deepmodeling:developfrom
mohanchen:2026-09-04-line1

Conversation

@mohanchen

@mohanchen mohanchen commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Summary
This PR refactors the occupation-number (Occupy) parallel reduction path in source_estate, exploring a new module_parallel domain-based design and finally settling back on the legacy Parallel_Reduce approach after the new design proved problematic.

What was done
Decouple Occupy from global parameter.h state (4f90c97, 4218bbe)
Removed the parameter.h dependency from occupy.cpp/write_elecstat_pot.cpp; nspin, npool, and Input_para are now passed explicitly through the interfaces, reducing hidden global coupling.
Test hygiene (53ec47a)
Removed the #define private public hack from the occupy unit test and renamed it to test_occupy.cpp, following the repository test-naming convention.
Attempted migration to module_parallel domain objects (d185d2e, 4e3f2d0, a5bd1bc, e3c3f96, 86499a2, ec6f175)
Replaced Parallel_Reduce calls in the occupy chain with a two-step reduction based on new domain objects: ParaKmeshWorld (cross k-pool) and ParaBdiffKsameWorld (cross band groups), plus a reduce-only para_bridge.
Fixed several issues along the way: ParaWorld ODR violation, restoring the bndpar dimension in ParaKmeshWorld reductions (fixing scf_bpcg), guarding empty pools, and correcting cross-pool reduction for uneven k-pool layouts.
Renamed the "bgroup" domain to bdiff_ksame for clearer k-pool vs band-pool terminology.
Reverted the occupy chain back to legacy Parallel_Reduce (58e8f8e)
The new domain-based parallel design turned out to be flawed for this use case (cross-pool reduction semantics with uneven k-pool layouts are error-prone), so occupy.cpp/.h, elecstate_tools.cpp, and test_occupy.cpp were restored to the legacy Parallel_Reduce::reduce_double_allpool(npool, GlobalV::NPROC_IN_POOL, x) style.
The explicit nspin/npool parameter passing from step 1 is kept; only the reduction mechanism is reverted.
The new module_parallel domain files (para_kmesh_world, para_bdiff_ksame_world, para_bridge) are retained, as they may be used by other modules, but the occupy chain no longer references them.
Net effect
Occupy interfaces are now free of parameter.h global dependencies (explicit nspin/npool arguments).
Occupation/energy reductions use the well-tested legacy all-pool reduction.
New module_parallel domain infrastructure is available in source_base with MPI tests, but not wired into source_estate.
Verification
cmake --build build --target MODULE_ESTATE_elecstate_occupy -j 8 — builds successfully.
ctest --test-dir build -V -R MODULE_ESTATE_elecstate_occupy — 25/25 tests passed.

Summary2
Establish images_world and esolver_world level MPI parallel infrastructure as the first step toward replacing the legacy global-communicator architecture with the new ParaCollection framework. With nimage=1 (the only supported value), this change is behavior-neutral.

Changes
New INPUT parameter nimage (input_parameter.h, read_inp_sys.cpp)

Declares int nimage = 1 with validation: nimage < 1 rejected as invalid; nimage > 1 rejected with "not implemented yet", reserving the parameter for future path-based methods (e.g. NEB).
Documented in docs/parameters.yaml and docs/advanced/input_files/input-main.md.
Global ParaCollection holder (para_worlds_global.h, .cpp)

init_global_para_worlds(nproc, my_rank, nimage): one-time initialization that splits MPI_COMM_WORLD by image id into per-image esolver_world plus the cross-image images_world via Parallel::split_images. Stored in a function-static unique_ptr with an initialization latch — written once at startup, read-only afterwards (no mutable cross-module workflow state).
global_para_worlds(): read-only access, WARNING_QUIT if not initialized.
reset_global_para_worlds_for_test(): test-only reset.
Header uses a forward declaration to minimize include dependencies.
Driver wiring (driver.cpp)

Calls init_global_para_worlds() in Driver::reading() right after Input_Conv::Convert() and before the legacy split_diag_world/split_grid_world/init_pools calls. With nimage=1, esolver_world is congruent to MPI_COMM_WORLD, so the legacy decomposition stays bit-identical. No downstream code reads the new domains yet.
A TODO marks the future work: re-base the legacy decomposition onto esolver_world once nimage > 1 is enabled.
Unit test (test_para_worlds_global.cpp)

Covers the state machine: init registers esolver/images domains with trivial sizes, reset clears the latch so a fresh initialization succeeds, and out-of-range image counts abort via WARNING_QUIT.
Wired through AddTest with LIBS base device and an explicit source-root include directory.
Verification
make -j 30 in build_max_para_test: abacus_max_para and all unit-test targets build.
ctest -R MODULE_MAIN_para_worlds_global: 3/3 passed.
PW SCF case 001_PW_UPF100_Al (2 MPI ranks): FINAL_ETOT_IS -57.0219080996 eV vs reference -57.02190810 eV (within floating-point tolerance).
nimage 2 correctly rejected during INPUT processing via WARNING_QUIT.
agent_governance_check.py: the only WARNINGs introduced by this change (header dependency, INPUT documentation linkage) are resolved. Global dependency budget net delta = -175.
Follow-up Steps (not in this PR)
Phase 0: Align the new split_diag_world/split_grid_world with the legacy transpose-coloring semantics (restore dcolor, verify MPI_Comm_compare congruence), with a side-by-side test.
Phase B: Migrate downstream readers from POOL_WORLD/KP_WORLD/DIAG_WORLD/GRID_WORLD to global_para_worlds().find(tag), one domain per commit.
Phase C: Replace the legacy Parallel_Global::split_* calls with setup_para_worlds, then delete the legacy functions.

abacus_fixer added 4 commits September 4, 2026 10:48
…l explicitly

Remove the `#include "source_io/module_parameter/parameter.h"` from
occupy.cpp and replace all PARAM references with explicit parameters:
- iweights gains `const int nspin` (replaces PARAM.inp.nspin)
- gweights/efermig/sumkg gain `const int npool` (replaces
  GlobalV::KPAR * PARAM.inp.bndpar computed inside sumkg)
- gweights weight loop bound changes from PARAM.globalv.nbands_l to
  the existing `nband` parameter (equivalent in production)

Update the sole production caller (elecstate_tools.cpp) to pass the
new arguments, extracting nspin/npool as locals so the global
dependency budget net decreases by 7.  Update the unit test to drop
the parameter.h include hack and adjust the Gweights expectation to
the exact half-filled single-band solution now that the weight loop
actually executes.  Drop `parameter` from the test target link list.
…ccupy.cpp

Promote efermig, sumkg, wgauss, w1gauss from private to public in
occupy.h so the unit test can call them without the access hack.
Rename elecstate_occupy_test.cpp to test_occupy.cpp per the
test_<module_name>.cpp naming convention and update CMakeLists.txt.
Migrate occupy.cpp and elecstate_tools.cpp from the legacy
Parallel_Reduce::reduce_double_allpool / reduce_max / reduce_min
to the new module_parallel ParaKmeshWorld API:

- Add reduce_across_pools / reduce_max_across_pools /
  reduce_min_across_pools to ParaKmeshWorld (Allreduce on the
  inter-pool communicator KP_WORLD, no-op when kpar==1).
- Add a reduce-only ParaKmeshWorld() default constructor and a
  no-arg make_kmesh_world() bridge overload for call sites like
  calEBand that have no k-point information to pass.
- occupy.h/cpp: replace npool parameter with const ParaKmeshWorld&,
  remove #include parallel_reduce.h, remove GlobalV::NPROC_IN_POOL.
- elecstate_tools.cpp: calEBand and calculate_weights now construct
  kmesh via the bridge; delete the old npool/bndpar local variable.

Fix a latent ODR violation in ParaWorld that caused a free() crash
in test_occupy: the MPI_Comm comm_ member only existed under
#ifdef __MPI, so the class layout differed between translation units
compiled with and without __MPI. Replace with a void* opaque handle
(memcpy round-trip, layout-identical in both builds).
@mohanchen mohanchen added Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0 labels Sep 4, 2026
…a to ctrl_output_fp

- write_elecstat_pot: add nspin, efield_flag, dip_cor_flag, imp_sol, two_fermi
  as explicit parameters instead of reading PARAM directly
- ctrl_output_fp: add const Input_para& inp parameter to replace PARAM.inp usage
- esolver_fp: pass PARAM.inp at the call site

@Critsium-xy Critsium-xy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reduction-domain replacement introduces two numerical correctness regressions. They fully account for the 5 failing CI cases.

Blocking

1. The BNDPAR dimension is no longer reduced

The old code reduced over npool = GlobalV::KPAR * PARAM.inp.bndpar on MPI_COMM_WORLD:

const int npool = GlobalV::KPAR * PARAM.inp.bndpar;
Parallel_Reduce::reduce_double_allpool(npool, GlobalV::NPROC_IN_POOL, value);

The new ParaKmeshWorld::reduce_across_pools (para_kmesh_world.cpp:104) reduces only on KP_WORLD, and KP_WORLD = kpar_group.inter_comm (parallel_global.cpp:260) spans the KPAR dimension only — it does not include band groups.

ekb.create(nk_in, PARAM.globalv.nbands_l) (elecstate.cpp:39), and under BPCG nbands_l = nbands / bndpar, so each band group holds only its own shard of the bands. As a result f_en.eband, f_en.demet, the electron count in Occupy::sumkg, and the ef max in iweights all keep only the contribution of the local band group.

When kpar == 1 && bndpar > 1, reduce_across_pools returns early and the reduction disappears entirely — the old code had npool = 2 there and did reduce.

Every failing case has bndpar 2 in its INPUT:

Case Configuration
212_PW_USPP_BPCG kpar 1, bndpar 2; etot deviation -11.87 Ry, stress deviation 58298
211_PW_BPCG_KB_OCP_CHG kpar 2, bndpar 2
087_PW_get_pchg_kpar_bndpar / 089_PW_get_wf_kpar_bndpar kpar 2, bndpar 2
scf_bpcg (CUDA 11_PW_GPU) kpar 1, bndpar 2

split_pools() in para_setup.h already produces bgroup_bp (the BP_WORLD domain). The reduction has to span both the kmesh and the band domain, or use an explicit all-pools domain. Note also that the comment at elecstate_tools.cpp:182 says "demet is accumulated independently on every k-point and band partition", yet only the k-direction reduction follows.

2. Reduction is silently skipped when KP_WORLD == MPI_COMM_NULL for an uneven KPAR split

divide_group_comm(KPAR, false) (parallel_global.cpp:243) permits an uneven split. In that case is_even == false, inter_comm is never created, and KP_WORLD = MPI_COMM_NULL.

make_kmesh_world (para_bridge.cpp:29, :50) requires KPAR > 1 && KP_WORLD != MPI_COMM_NULL; otherwise it falls back to a serial domain with kpar_ = 1 and the reduction becomes a no-op. The old reduce_double_allpool went through MPI_COMM_WORLD and was correct for uneven splits (each pool divides by its own NPROC_IN_POOL before the sum).

The failing case 007_PW_UPF201_USPP_Fe uses kpar 3 and CI runs 4 processes; 4 % 3 != 0 hits exactly this path.

The fallback disguises a broken configuration as a valid one: kpar_ = 1 asserts "there are no pools to reduce over" when there are in fact 3. Parallel_Grid::reduce_across_pools (parallel_grid.cpp:155) handles the same situation with an allgather + INT_BGROUP fallback and can serve as a reference. At the very least this should WARNING_QUIT when the reduction cannot be performed, rather than silently producing a wrong energy.

Non-blocking

3. The default-constructed ParaKmeshWorld is a UB trap. The default constructor (para_kmesh_world.cpp:18) leaves nks_pool_ / whichpool_ / startpro_pool_ empty, while the MPI branch of make_kmesh_world() (para_bridge.cpp:32) passes nkstot=0 through distribute_kpoints() and ends up with kpar_-sized all-zero vectors. That is two different internal invariants for the same reduce-only semantics: the former makes max_nks_pool() call *std::max_element(...) on an empty vector (UB), the latter returns 0. Suggest a has_distribution_ flag plus asserts at the entry of pool_collection / gather_kvec / max_nks_pool / which_pool.

4. The void* comm_ ODR fix is only half a fix. source_estate/test/CMakeLists.txt starts with abacus_disable_feature_definitions(__MPI), so the test TU is compiled without __MPI yet links an __MPI-compiled base — genuinely UB. But switching to void* only unifies the layout: ParaWorld::comm(), ParaWorld(tag, comm) and ParaKmeshWorld(comm, ...) remain inside #ifdef __MPI, the two TUs still see different class definitions, and the ODR violation still stands. The real fix is to compile the test with the same macro set, or not to link the MPI-built base. Separately, the memcpy + static_assert pair would be clearer as a fixed-size aligned byte buffer, and pulling <cstring> into a widely included header like para_world.h is a governance warning.

5. No tests for the new reduction interface. reduce_across_pools / reduce_max_across_pools / reduce_min_across_pools are the core addition of this PR. module_parallel/test/para_setup_mpi_test.cpp is an existing precedent for MPI tests; a 2-pool case would catch both issues 1 and 2.

6. Doc comments do not match the actual semantics. In para_kmesh_world.h, "Replaces Parallel_Reduce::reduce_double_allpool" does not hold for bndpar > 1, and "no normalization by pool size is needed" is only true for the kpar dimension. Please update these along with the fix for issue 1.

7. Repeated construction on a hot path. elecstate_tools.cpp:110 and :132 call make_kmesh_world(klist->get_nkstot(), nspin) on every SCF iteration, and distribute_kpoints() allocates an nkstot-long whichpool_ plus three kpar-long vectors. calEBand in the same file uses the reduce-only overload instead. Since only the reduction is needed, both sites can use the reduce-only version; better still, pass the domain object down from the caller.

8. occupy.h moves efermig / sumkg / wgauss / w1gauss from private to public purely to replace the #define private public hack. Dropping the hack is worthwhile, but widening the public API in exchange should be called out in the PR description, or replaced with a friend test fixture.

Side note

Changing the gweights loop bound from PARAM.globalv.nbands_l to the nband parameter is a genuine fix: the previous Gweights unit test never executed the loop body because nbands_l defaults to 0, so wg / demet kept their preset values and the test was vacuous. The new test values (nelec=0.5, ef=-1.0, wg=0.5, demet=σ·w1gauss(0,0)) check out. The only production call site passes nbands = ekb.nc, and ekb is allocated with nbands_l, so the change is equivalent there.

Suggested order of fixes

  1. Make the reduction span both the kmesh and band domains (reuse bgroup_bp from split_pools), fixing the 4 bndpar cases.
  2. Handle KP_WORLD == MPI_COMM_NULL for uneven splits — either fall back the way Parallel_Grid does, or fail loudly — fixing 007_PW_UPF201_USPP_Fe.
  3. Add MPI unit tests covering both paths.
  4. Clean up items 3–8.

abacus_fixer added 20 commits September 4, 2026 16:35
Commit d185d2e migrated the occupation reductions from
Parallel_Reduce::reduce_double_allpool(KPAR*bndpar, ...) to
ParaKmeshWorld, but the new domain only Allreduced over KP_WORLD and
returned early when kpar==1, silently dropping the band-parallel
(bndpar>1) contributions. This broke tests/11_PW_GPU/scf_bpcg
(kpar=1, bndpar=2, gaussian smearing): the occupation sum was
accumulated from the local band shard only (sumkup=24 instead of 28),
failing the smearing consistency check.

- ParaKmeshWorld gains a bndpar member with npool() = kpar*bndpar;
  reduce_across_pools restores the legacy semantics: divide by the
  pool size (nproc/npool) and Allreduce over MPI_COMM_WORLD. The
  max/min reductions span all pools likewise. npool()==1 is a no-op.
- para_bridge derives bndpar from the pool layout
  (bndpar = NPROC / (KPAR * NPROC_IN_POOL)) to avoid depending on the
  INPUT-parameter module from source_base, and builds the kmesh domain
  on MPI_COMM_WORLD whenever KPAR*bndpar > 1.
- Add MPI unit test test_para_kmesh_world_mpi.cpp (mpirun -np 4)
  covering the previously missing bndpar=2 band-group reduction path,
  the kpar=2 path, and the single-pool no-op; wired via add_executable
  plus a .sh runner so no single-process CTest entry is created.
- Update para_collection_mpi_test.cpp for the new constructor.

Verified: cmake build + OMP_NUM_THREADS=1 ctest -R
"para_kmesh_world|para_collection|elecstate_occupy|elecstate_base"
-- 7/7 passed.
Two follow-ups from review of the bndpar reduction fix:

- calculate_weights called make_kmesh_world(nkstot, nspin) on every
  SCF iteration, rebuilding the k-point distribution arrays although
  only the cross-pool reductions are used. Switch both branches
  (iweights and gweights) to the reduce-only make_kmesh_world()
  overload, matching what calEBand already does.
- max_nks_pool() dereferences an empty vector on reduce-only domains;
  add an assert so the invalid query fails loudly instead of hitting
  undefined behavior.

Verified: cmake build + OMP_NUM_THREADS=1 ctest -R
"para_kmesh_world|para_collection|elecstate_occupy|elecstate_base"
-- 7/7 passed.
…e_cell

Relocate cube_io.h, read_cube.cpp, write_cube.cpp, output_log.h/cpp,
write_orb_info.h/cpp, and write_pao.h/cpp into source_cell to match
their UnitCell-centric semantics. Update all include paths, CMake
registrations, Makefile.Objects groups, and test CMakeLists.txt
entries accordingly. Global state cleanups inside the moved files are
deferred to a follow-up change.
upstream/develop renamed ModuleBase::world_communication_domain() to
world_comm_domain() in parallel_cell; update the three test_parallel
call sites merged in from upstream so the tree compiles again.
The term "pool" was overloaded: MY_POOL refers to a k-pool (the KPAR
split, done first and independent of bndpar), while NPROC_IN_POOL /
RANK_IN_POOL / POOL_WORLD refer to the (k-pool x band-group) cell
created by the later BNDPAR split. Document the two-level split order
and each communicator's exact semantics, including that KP_WORLD is
MPI_COMM_NULL for uneven k-pool sizes. Comment-only change.
Add ParaBgroupWorld::reduce_across_bgroups, summing a scalar across the
BNDPAR band groups of one k-pool on BP_WORLD (bdiff_ksame). BPCG shards
the band range across band groups, so each process only accumulates its
own band window; this combines those partial sums before the k-pool
reduction. Also add the make_bgroup_world bridge from the old INT_BGROUP
/ BP_WORLD globals, guarded against uninitialized layouts. Pure
addition, no caller yet.
ParaKmeshWorld::reduce_across_pools divided each partial sum by
nproc/npool (an integer average pool size) before a world-wide
Allreduce. With uneven k-pools (e.g. nproc=4, kpar=3 as in
007_PW_UPF201_USPP_Fe) that division collapsed to 1 and double-counted
the front pool, corrupting the electron count, the Fermi level and thus
the total energy (deviation ~8.8 Ry).

Make the k-mesh domain a pure k-pool topology, independent of bndpar:
- reduce_across_pools now lets only the first process of each k-pool
  contribute (one contribution per pool, no division), correct for both
  even and uneven pool sizes.
- The band dimension moves out of the k-mesh domain: callers
  (Occupy::sumkg, calEBand, calculate_weights/demet) now run
  ParaBgroupWorld::reduce_across_bgroups first, then the k-pool
  reduction. This preserves the bpcg behavior previously folded into
  npool() = kpar * bndpar.
- reduce_max/min_across_pools stay world-wide: max/min are idempotent,
  and band-parallel shards see different eigenvalue windows, so the
  Fermi bounds must be extremized across both dimensions.
- Fix the member-order bug where distribute_kpoints() derived the
  per-pool first-rank table from an uninitialized nproc_ (always 1).
- Drop the bndpar constructor argument and the bndpar_from_layout
  reverse derivation from the bridge.

Update the MPI unit tests: the bndpar=2 case now exercises the two-step
protocol, and a new uneven k-pool case (nproc=4, kpar=3) asserts exactly
one contribution per pool. Verified: 4/4 MPI tests, 7 serial kmesh,
2 bgroup, 6 collection, 25 occupy tests pass.
…allel

The bgroup name is misleading: it does not say which two axes the domain
connects. Rename the new module_parallel band-group domain after its tag
bdiff_ksame (different band groups, same k), matching ParaTag:

  para_bgroup_world.{h,cpp}   -> para_bdiff_ksame_world.{h,cpp}
  ParaBgroupWorld             -> ParaBdiffKsameWorld
  make_bgroup_world()         -> make_bdiff_ksame_world()
  reduce_across_bgroups()     -> reduce_across_bdiff_ksame()

Legacy globals INT_BGROUP / BP_WORLD and the GlobalV::MY_BNDGROUP-style
member names are the old parallel-layer API and are left unchanged; the
new domain is only a self-contained bridge over them.
The new parallel design based on module_parallel domain objects
(ParaKmeshWorld/ParaBdiffKsameWorld two-step reduction) is flawed,
so revert the occupy chain to the legacy
Parallel_Reduce::reduce_double_allpool(npool, NPROC_IN_POOL, x) style.

- occupy.cpp/.h: restore npool-based iweights/gweights/efermig/sumkg
- elecstate_tools.cpp: restore allpool reduction for eband/demet
- test_occupy.cpp: drop ParaKmeshWorld argument from test calls

The module_parallel domain files are kept for other users.
Verified: cmake --build build --target MODULE_ESTATE_elecstate_occupy;
ctest -R MODULE_ESTATE_elecstate_occupy (25/25 passed).
Declare int nimage = 1 in Input_Param and register it in read_inp_sys.cpp
with documentation and validation. nimage < 1 is rejected as invalid;
nimage > 1 is rejected as not-yet-implemented, reserving the parameter for
future path-based methods (e.g. NEB) that will run one esolver instance per
image on a dedicated esolver_world communicator.

This is a behavior-neutral parameter addition: the default nimage=1 keeps
all existing execution paths unchanged. Verified with `make -j 30` in
build_max_para_test (abacus_max_para built) and
`abacus_max_para -h nimage` printing the registered help entry.
Introduce source_main/para_worlds_global.{h,cpp} providing:
- init_global_para_worlds(nproc, my_rank, nimage): one-time initialization
  that splits MPI_COMM_WORLD by image id via Parallel::split_images and
  registers the esolver and images domains in a process-wide ParaCollection.
- global_para_worlds(): read-only access, WARNING_QUIT if not initialized.
- reset_global_para_worlds_for_test(): test-only reset.

The storage is a function-static unique_ptr with an initialization latch:
written once at startup, read-only afterwards, so it does not add mutable
cross-module workflow state. The new source file is wired into the existing
`driver` OBJECT library in source/CMakeLists.txt.

The module is compiled but not yet called; the driver wiring is the next
step. Verified with `make -j 30` in build_max_para_test (driver and
abacus_max_para built).
Add source_main/test/test_para_worlds_global.cpp covering the holder state
machine: init registers the esolver and images domains with trivial sizes,
reset clears the latch so a fresh initialization succeeds, and out-of-range
image counts (nimage < 1 or nimage > nproc) abort via WARNING_QUIT.

Move the nimage/nproc argument validation out of the __MPI block in
para_worlds_global.cpp: it is MPI-independent and must also protect serial
builds (this was caught by the new death tests, which failed to die under
the non-MPI test configuration).

Register the test directory in source/CMakeLists.txt (source_main has no
CMakeLists of its own) and add the test CMakeLists.txt wiring through AddTest
with LIBS base device plus an explicit source-root include directory.

Verified: ctest -R MODULE_MAIN_para_worlds_global passes and a full
`make -j 30` builds abacus_max_para cleanly.
Call Parallel::init_global_para_worlds() in Driver::reading() right after
Input_Conv::Convert() and before the legacy split_diag_world/split_grid_world/
init_pools decomposition. This establishes the image-level communication
domains (esolver_world per image, cross-image images_world) and stores them
in the process-wide ParaCollection.

For the only supported nimage = 1 the esolver world is congruent to
MPI_COMM_WORLD, so the legacy decomposition still performed on
MPI_COMM_WORLD is bit-identical: no downstream code reads the new domains
yet. A TODO marks the future work of re-basing the legacy splits onto the
esolver world once nimage > 1 (NEB-style multi-image runs) is enabled.

Verified in build_max_para_test (`make -j 30`):
- PW SCF case 001_PW_UPF100_Al on 2 MPI ranks converges normally with
  FINAL_ETOT_IS -57.0219080996 eV vs reference -57.02190810 eV.
- nimage 2 is rejected during INPUT processing with
  "nimage > 1 is not implemented yet" via WARNING_QUIT.
- Replace the para_collection.h include in para_worlds_global.h with a
  forward declaration of ParaCollection; the header only exposes references.
  Move the complete-type include to the .cpp and the test that need it.
- Register nimage in docs/parameters.yaml and
  docs/advanced/input_files/input-main.md (including the table of contents),
  satisfying the INPUT documentation linkage requirement for the new
  parameter.

Verified: abacus_max_para and MODULE_MAIN_para_worlds_global build and the
unit test passes.
State expected token cost before non-trivial actions and prefer the
cheapest path (direct Read/Grep over sub-agents) to avoid overspending.
Resolve conflict in write_orb_info_test.cpp by adopting upstream's
new include path (source_io/module_output/write_orb_info.h) and
dropping the #define private public test access hack.
upstream moved the header to source_io/module_output/ in their tree,
but that path did not land in this merge; keep the original
source_cell/ path while still dropping the #define private public hack.
The Makefile-based CI build (Makefile.Objects) was missing the new
para_worlds_global translation unit, causing an undefined reference
to Parallel::init_global_para_worlds at link time. CMake build was
already correct.
Resolve conflicts in ctrl_output_fp and esolver_fp:
- Adopt upstream's inp_ member and Input_para as 2nd argument of
  ctrl_output_fp; drop the duplicate trailing inp parameter added by
  this branch.
- Keep this branch's extended write_elecstat_pot call (nspin,
  efield_flag, dip_cor_flag, imp_sol, two_fermi) matching its extended
  signature in write_elecstat_pot.h/.cpp.
- esolver_fp.cpp calls ctrl_output_fp via *this->inp_.

@Critsium-xy Critsium-xy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One correctness issue in the new k-mesh domain: the defect it claims to fix in the legacy all-pool reduction does not exist, and two new comments assert that it does.

Parallel_Reduce::reduce_double_allpool (source/source_base/parallel_reduce.cpp:137) divides by nproc_in_pool, and callers pass GlobalV::NPROC_IN_POOL. That value is not an average: divide_pools sets it from bndpar_group.nprocs_in_group (parallel_global.cpp:296), and divide_mpi_groups increments procs_in_group for the leading groups that absorb the remainder (parallel_global.cpp:279-281). Each process therefore divides by the true size of its own pool, and the world-wide MPI_Allreduce sums exactly one contribution per pool.

For the nproc=4 / kpar=3 layout cited in the test, NPROC_IN_POOL is 2 on ranks 0-1 and 1 on ranks 2-3, so the legacy path computes 10/2 + 10/2 + 20/1 + 30/1 = 60 - the same value the new reduce_across_pools asserts. No double counting, no corrupted electron count or Fermi level.

Since this is the only stated correctness advantage of the new reduction, please drop or reword both comments; as written they would send someone off to fix a bug that is not there.

// count each pool exactly once even though the pools are uneven:
// the legacy average-pool-size division (4/3 = 1) double-counted
// pool 0 here and corrupted the electron count / Fermi level.
const double pool_sum = (my_pool == 0) ? 10.0 : ((my_pool == 1) ? 20.0 : 30.0);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This claim is incorrect. NPROC_IN_POOL is the local pool size, not an average: divide_mpi_groups bumps procs_in_group for the leading groups that take the remainder (parallel_global.cpp:279-281). In this exact layout it is 2 on ranks 0-1 and 1 on ranks 2-3, so the legacy path gives 10/2 + 10/2 + 20/1 + 30/1 = 60, identical to what this test asserts. Pool 0 is not double counted.

* zero, so a single world-wide MPI_Allreduce yields the sum of the
* per-k-pool partial sums. Correct for uneven k-pool sizes and free
* of the legacy normalization division (which divided by an average
* pool size and double-counted pools of uneven layouts).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as the test comment: there is no "average pool size" division to be free of. reduce_double_allpool divides by each process's own NPROC_IN_POOL, which already accounts for uneven k-pools. Please reword - this is currently the only documented correctness argument for the new API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants